Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-29068: Report percentFilesLocalPrimaryRegions metric #6596

Merged

Conversation

charlesconnell
Copy link
Contributor

@charlesconnell charlesconnell commented Jan 10, 2025

If you are running a table with region replicas, the secondary replicas usually cannot achieve 100% locality. Currently a RegionServer reports two locality metrics:

  • percentFilesLocal
  • percentFilesLocalSecondaryRegions

Both of these will be less than 100% if region replicas are enabled. The best we can do is get 100% locality for the primary replicas, but there is currently no way to see if we've achieved it.

I propose introducing the new metric percentFilesLocalPrimaryRegions, which will close this gap.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 28s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
_ master Compile Tests _
+0 🆗 mvndep 0m 13s Maven dependency ordering for branch
+1 💚 mvninstall 3m 28s master passed
+1 💚 compile 4m 36s master passed
+1 💚 checkstyle 1m 0s master passed
+1 💚 spotbugs 2m 27s master passed
+1 💚 spotless 0m 55s branch has no errors when running spotless:check.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 10s Maven dependency ordering for patch
+1 💚 mvninstall 3m 30s the patch passed
+1 💚 compile 4m 11s the patch passed
+1 💚 javac 4m 11s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 0m 54s the patch passed
+1 💚 spotbugs 2m 59s the patch passed
+1 💚 hadoopcheck 12m 47s Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚 spotless 0m 54s patch has no errors when running spotless:check.
_ Other Tests _
+1 💚 asflicense 0m 22s The patch does not generate ASF License warnings.
47m 41s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6596/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #6596
JIRA Issue HBASE-29068
Optional Tests dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname Linux 167113b03f97 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 4860ace
Default Java Eclipse Adoptium-17.0.11+9
Max. process+thread count 83 (vs. ulimit of 30000)
modules C: hbase-hadoop-compat hbase-server U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6596/1/console
versions git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Copy link
Contributor

@rmdmattingly rmdmattingly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll wait for tests to pass for final approval, but this looks good to me

Comment on lines -320 to +326
"The percent of HFiles used by secondary regions that are stored on the local hdfs data node.";
"The percent of HFiles used by secondary regions that are stored on the local hdfs data node. "
+ "This is not likely to reach 100%";
Copy link
Contributor

@rmdmattingly rmdmattingly Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it's unlikely, but wouldn't it be possible to achieve 100% locality for both primary and secondary replicas if your number of replicas is less than or equal to your hdfs replication factor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically that's possible. I say it's unlikely because something would need to ensure that all the block replicas for a secondary region end up co-located with the region, and nothing I'm aware of would do that. Compactions won't help. Internally at hubspot we have the locality healer, but today even that doesn't know about secondary regions, although I plan on that eventually.

Copy link
Contributor

@rmdmattingly rmdmattingly Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The balancer is aware of locality — I wonder to what extent LocalityBasedCandidateGenerator could maintain secondary replica locality. I haven't looked at the implementation with replicas in mind, but it might already balance secondary replicas based on locality to some extent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I would still argue that that isn't likely to be very helpful. The region balancer needs to put a region on exactly one server, but in the absence of some helpful force, it's unlikely that all the blocks for any given region exist on a single DataNode. The region balancer can only do as well as the random block distribution allows. Before we had the locality healer at hubspot, we experienced this difficultly all the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 39s Docker mode activated.
-0 ⚠️ yetus 0m 3s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗 mvndep 0m 12s Maven dependency ordering for branch
+1 💚 mvninstall 4m 9s master passed
+1 💚 compile 1m 31s master passed
+1 💚 javadoc 0m 55s master passed
+1 💚 shadedjars 7m 11s branch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 12s Maven dependency ordering for patch
+1 💚 mvninstall 3m 48s the patch passed
+1 💚 compile 1m 32s the patch passed
+1 💚 javac 1m 32s the patch passed
+1 💚 javadoc 0m 50s the patch passed
+1 💚 shadedjars 6m 51s patch has no errors when building our shaded downstream artifacts.
_ Other Tests _
+1 💚 unit 0m 48s hbase-hadoop-compat in the patch passed.
-1 ❌ unit 230m 5s /patch-unit-hbase-server.txt hbase-server in the patch failed.
269m 40s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6596/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR #6596
JIRA Issue HBASE-29068
Optional Tests javac javadoc unit compile shadedjars
uname Linux c1b0a71c3f4d 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 4860ace
Default Java Eclipse Adoptium-17.0.11+9
Test Results https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6596/1/testReport/
Max. process+thread count 4730 (vs. ulimit of 30000)
modules C: hbase-hadoop-compat hbase-server U: .
Console output https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6596/1/console
versions git=2.34.1 maven=3.9.8
Powered by Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@charlesconnell
Copy link
Contributor Author

Test failure appears unrelated

Copy link
Contributor

@rmdmattingly rmdmattingly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can ship this tomorrow

@charlesconnell
Copy link
Contributor Author

@rmdmattingly reminder about this, when you have a chance

@rmdmattingly
Copy link
Contributor

Thanks for the reminder

@rmdmattingly rmdmattingly merged commit e29fc04 into apache:master Jan 16, 2025
1 check failed
@rmdmattingly rmdmattingly deleted the HBASE-29068/primary-region-locality branch January 16, 2025 13:45
rmdmattingly pushed a commit that referenced this pull request Jan 16, 2025
rmdmattingly pushed a commit that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants